Skip to content

feat(receive): redesign receive flow with improved sheets and toasts#480

Open
sahilc0 wants to merge 14 commits intowt-navbar-root-only-20260325from
wt-receive-on-navbar-20260326
Open

feat(receive): redesign receive flow with improved sheets and toasts#480
sahilc0 wants to merge 14 commits intowt-navbar-root-only-20260325from
wt-receive-on-navbar-20260326

Conversation

@sahilc0
Copy link
Copy Markdown
Contributor

@sahilc0 sahilc0 commented Mar 26, 2026

Summary

BLOCKED BY: Kukks' LNURL work. This PR redesigns the receive flow to show the QR code immediately (skipping the amount screen), but we should not merge until we have zero-invoice Lightning via LNURL. Without that, showing the QR first means no Lightning option unless the user manually sets an amount. The full UX improvement depends on LNURL landing first.

  • Receive layout: QR centered vertically with actions pinned to bottom for balanced spacing
  • Sheet modal (Vaul-inspired): Visual elevation via Ionic CSS custom properties (shadow, border, overflow), wider pill handle, no close button
  • Toast (Sonner-inspired): Custom toast system replacing useIonToast across all call sites — dark bg, top-positioned, 250ms ease-out animations, reduced-motion compliant
  • Copy sheet: Reordered addresses (Unified > Lightning > Arkade > Bitcoin), proper header styling, accessible 44px copy buttons with aria-labels
  • Accessibility: :focus-visible instead of :focus (no purple flash on tap), semantic <button> elements, touch-action: manipulation

Dependency

This PR is blocked by Kukks' LNURL implementation. The current receive flow shows the QR immediately without an amount, which means Lightning is unavailable until the user sets an amount (500+ sats). Once LNURL lands, we'll have zero-invoice Lightning support and the "show QR first" UX will work end-to-end.

Do not merge until LNURL is available.

Base branch

This PR is based on wt-navbar-root-only-20260325 (#474 — navbar only on root pages) to avoid navbar interference with the receive sheet modals.

Test plan

  • QR centered vertically, actions pinned to bottom on various screen heights
  • Sheet modals show visible elevation (shadow + border)
  • Toast appears at top with dark bg, auto-dismisses in 2s
  • Copy buttons have 44px tap targets, no purple focus flash on tap
  • Address order in copy sheet: Unified, Lightning (when available), Arkade, Bitcoin
  • Amount sheet header matches copy sheet styling
  • Dark mode: toast slightly lighter gray, sheet has stronger shadow
  • Reduced motion: no animation on toast enter/exit

sahilc0 added 4 commits March 26, 2026 14:48
- Skip amount screen, show QR immediately on receive
- Custom SVG QR with circular dots, rounded finder patterns, Arkade logo center
- Logo uses brand color (--logo-color), adapts to light/dark theme
- QR dots animate in with staggered ripple on value change
- Buttons inline with content (not fixed footer) — scrollable layout
- Amount entry and copy address via bottom sheet modals
- SheetModal accounts for pill navbar clearance
- Content noFade prop to disable scroll mask on receive screen
- Fetch addresses on mount (merged from Amount screen)
- Sheet modal: iOS-inspired with shadow, handle bar, rounded close button
- Sheet z-index above navbar (z-index: 200), backdrop dismiss enabled
- Haptics on sheet close
- Amount keyboard bypasses sheet on save — goes straight to QR
- Button label: "Add amount" / "Edit amount" instead of sats number
- Clear amount option in sheet to reset to no-amount QR
- "500 sats min for Lightning" hint tightly coupled under QR
- "Requesting X sats" text positioned closer to QR
Receive flow layout:
- Center QR vertically in available space, pin actions to bottom
- Add breathing room with 1.5rem bottom padding on actions
- Reorder copy addresses: Unified > Lightning > Arkade > Bitcoin
- Rename BIP21 to "Unified", BTC address to "Bitcoin address"

Sheet modal (Vaul-inspired):
- Move visual styling to Ionic CSS custom properties (--background,
  --box-shadow, --overflow, --border-width) to escape shadow DOM
  overflow clipping that was hiding shadows on inner elements
- Remove close X button, use handle bar + backdrop dismiss
- Wider pill handle (40px), softer shadow, 16px border-radius
- Dark mode: stronger shadow + light border for elevation contrast

Toast (Sonner-inspired):
- New custom Toast component replacing useIonToast across 6 files
- Dark bg on light mode, slightly lighter gray on dark mode
- Top-positioned, 250ms ease-out enter/exit, full reduced-motion support
- 44px accessible copy buttons with aria-labels and touch-action

Accessibility:
- Copy icon buttons: div -> button with aria-label, 44px min tap target
- Focus highlight: :focus -> :focus-visible (no purple flash on tap)

Dev auto-init:
- Set authState to 'authenticated' directly instead of persisting
  encrypted key to localStorage (keeps dev NSEC in-memory only)
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 26, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • next-version

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 20da134a-a169-4723-9bf3-a6d5f1fb4012

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch wt-receive-on-navbar-20260326

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sahilc0 sahilc0 requested review from Kukks, bordalix and pietro909 March 26, 2026 13:51
@sahilc0
Copy link
Copy Markdown
Contributor Author

sahilc0 commented Mar 26, 2026

⚠️ Blocked: depends on LNURL

@Kukks — this PR redesigns the receive flow to show the QR code immediately (no amount screen first). However, this UX only makes sense once we have zero-invoice Lightning via LNURL.

Right now, without an amount set, there's no Lightning invoice in the QR — just Arkade + on-chain. That's a worse experience than the current flow where we prompt for amount first.

This should not be merged until LNURL support lands. Once it does, the receive flow will show a unified QR that supports Lightning without requiring the user to enter an amount upfront.

Keeping this as draft until then.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages bot commented Mar 26, 2026

Deploying wallet-bitcoin with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0062dad
Status: ✅  Deploy successful!
Preview URL: https://37f0d52c.wallet-bitcoin.pages.dev
Branch Preview URL: https://wt-receive-on-navbar-2026032.wallet-bitcoin.pages.dev

View logs

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages bot commented Mar 26, 2026

Deploying wallet-mutinynet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0062dad
Status: ✅  Deploy successful!
Preview URL: https://29bbc92d.arkade-wallet.pages.dev
Branch Preview URL: https://wt-receive-on-navbar-2026032.arkade-wallet.pages.dev

View logs

sahilc0 added 3 commits March 31, 2026 17:32
- Keep master's boot animation, error boundary, LNURL session
- Keep PR's redesigned receive UI with sheets and toasts
- Integrate LNURL amountless receive into new QR-first flow
- Replace removed Loading component with LoadingLogo
The noUserDefinedPassword() check races with the dev auto-init,
overriding authState to 'locked' and blocking the boot flow.
The fast auto-init from VITE_DEV_NSEC races with the boot animation
timing, causing the overlay to never dismiss.
@sahilc0 sahilc0 marked this pull request as ready for review April 1, 2026 20:01
@arkanaai
Copy link
Copy Markdown

arkanaai bot commented Apr 1, 2026

🔍 Review — feat(receive): redesign receive flow with improved sheets and toasts

Large PR: +11,336/-587 across 85 files. Absorbs multiple merged PRs from master (Docker, LNURL, Branta, error boundary, SW recovery, loading status, etc.) plus the receive redesign itself.


Architecture

The PR layers on wt-navbar-root-only-20260325 (#474) and integrates master, so it carries a lot of merged-in work. The receive-specific changes are well-structured:

  • useLnurlSession hook for SSE-based LNURL amountless receives
  • SheetModal redesign with Vaul-inspired elevation
  • Custom Toast system replacing useIonToast
  • QrCode component rewrite with SVG rendering and staggered animations

Security Review

LNURL session (useLnurlSession.ts) — Overall solid:

  • ✅ Auth token stored from session_created event, sent as Bearer header on invoice POSTs
  • ✅ Abort controller properly cancels on unmount
  • amountMsat validated as positive number before creating swap
  • ✅ Errors sent back to lnurl-server so payers get immediate feedback
  • ✅ SSE parser wraps JSON.parse in try/catch
  • eventType persisted outside while loop to handle cross-chunk SSE boundaries

One concern: tokenRef.current is never cleared between sessions except on full unmount (the finally block sets it to null). If the SSE stream reconnects (e.g. network blip) within the same mount, a stale token could be sent. This is low risk since the component unmounts on navigation, but worth noting.

Service worker hardening (merged from #487):

  • ✅ Zombie SW detection via PING/PONG before init
  • ✅ SW unregistration after exhausted retries
  • ✅ MessageBus timeout increased from 5s to 30s (appropriate for cold starts)
  • navigator.serviceWorker access guarded behind feature check

LNURL reverse swap flow in QrCode.tsx:

  • isAmountlessLnurl properly gated on: no amount set, no asset, lnurl server configured, connected, swaps initialized, no swap error
  • waitAndClaim runs in background with error logging
  • ⚠️ The handleInvoiceRequest callback captures recvInfo in its closure via the dependency array. If recvInfo changes between the initial render and when the invoice request arrives, the spread { ...recvInfo } may overwrite newer state. Consider using a ref for recvInfo in this callback, similar to how onInvoiceRequestRef is used in the hook itself.

ErrorBoundary (merged from #496):

  • ✅ Captures to Sentry with component stack
  • ✅ User-friendly fallback UI

UX Observations

  • BLOCKED BY LNURL — PR description clearly states this shouldn't merge until LNURL lands. Since the LNURL work (Add LNURL support for amountless Lightning receives #482) is now merged into this branch, the blocker appears resolved assuming the lnurl-server is deployed.
  • QR-first flow means Lightning is unavailable without amount unless LNURL session is active — the isAmountlessLnurl check handles this correctly.
  • Address ordering (Unified > Lightning > Arkade > Bitcoin) in copy sheet matches the hierarchy of payment rails.
  • package-lock.json (+8555 lines) — this is a npm lockfile in a pnpm project. Intentional? It's listed in the diff and would add significant repo bloat.

Cross-repo Impact

  • Depends on @arkade-os/sdk IVtxoManager interface and VtxoManager.getExpiringVtxos() — ensure ts-sdk exports are stable before merging.
  • @arkade-os/boltz-swap integration for LNURL reverse swaps uses the existing createReverseSwap flow — no new boltz-swap API surface needed.

Dev auto-init (VITE_DEV_NSEC)

The final commits fix race conditions where dev auto-init conflicted with boot animation and auth state checks. The approach (skip boot animation and auth check when VITE_DEV_NSEC is active) is pragmatic. Ensure VITE_DEV_NSEC is never set in production builds — the Vite env var prefix means it's compile-time, so this should be safe as long as .env.production doesn't define it.

Summary

Area Status
LNURL security ✅ Token auth, validation, abort handling
SW recovery ✅ Zombie detection, retry limits, timeout tuning
Error handling ✅ ErrorBoundary, BootError, Sentry filtering
Receive UX ✅ QR-first, sheet modals, accessibility
State management ⚠️ recvInfo closure in handleInvoiceRequest
package-lock.json ⚠️ npm lockfile in pnpm project — likely unintended

Verdict: Well-structured feature PR with good security practices on the LNURL flow. Two minor issues flagged. The npm lockfile should be removed before merge.

@sahilc0
Copy link
Copy Markdown
Contributor Author

sahilc0 commented Apr 2, 2026

@Kukks fixed the merge conflict — should be good to go now.

@arkanaai
Copy link
Copy Markdown

arkanaai bot commented Apr 2, 2026

🔍 Iterative Review — Merge Commit d718a15

This merge integrates wt-navbar-root-only-20260325 (the base branch) into the PR, bringing ~30 changed files. Key observations on the new delta:

Changes Worth Noting

Input.onChange type coercionInput now calls onChange(Number(v)) for type="number" inputs. This means onChange callers receive a number instead of a string. All asset screens (Burn, Mint, Reissue) have been updated to use useState<number> accordingly. This is a nice cleanup, but note that Number("") returns 0 — clearing the input field will silently produce 0 rather than undefined/empty. Verify this doesnt cause UX issues where a blank field should be distinguishable from 0.

prettyNumber robustness — Now correctly handles undefined/null/NaN (returns "0") and adds minimumFractionDigits. The old !num guard would treat 0 as falsy and return "0", which was technically correct but for the wrong reason. The new explicit check is better.

InputAmount asset guard — Changed from if (asset) return to if (asset?.assetId) return. This is a correctness fix — an object with no assetId would previously short-circuit the fiat conversion effect.

LoadingLogo triple-tap reveal — Status text and background are now hidden behind a 3-tap gesture. Clean implementation with proper timer cleanup. The 600ms window and 3-tap threshold feel reasonable.

Success screen navigation — All success screens (Send, Receive, Notes) now have a "Sounds good" button to navigate back to Pages.Wallet. Previously users had no explicit way to dismiss these screens. Good UX improvement.

Send form re-validationparseRecipient effect now depends on [recipient, isAssetSend] instead of just [recipient]. This correctly re-validates when switching between BTC and asset sends, catching the "Assets can only be sent to Arkade addresses" case. Also, amount/textValue are now cleared before the asset-specific branch, fixing a potential state ordering issue.

Mint control asset amount — Changed from unitsToCents(parseFloat(ctrlAmount) || 0, 0) to parseInt(ctrlAmount.toString()). Since control assets have 0 decimals, this is equivalent, but parseInt without a radix parameter defaults to base-10 for decimal strings — consider explicitly passing radix 10: parseInt(ctrlAmount.toString(), 10).

No Security Concerns

No key handling, signing, or protocol-level changes in this merge delta. All changes are UI/UX layer.

@bordalix
Copy link
Copy Markdown
Collaborator

bordalix commented Apr 3, 2026

Issue: don't use hardcoded 500 sats for min lightning, use value received from provider.
Fixed by 92f9499

@arkanaai
Copy link
Copy Markdown

arkanaai bot commented Apr 3, 2026

🔍 Arkana PR Review

Scope: UI redesign of the receive flow — QR code, sheet modals, toast system, accessibility.

Overview

Solid UX polish. The SVG QR code with animated dots and logo overlay is a nice upgrade from the GIF encoder. Custom toast system is cleaner than useIonToast. Sheet modal with pill handle matches modern mobile patterns. Good accessibility attention throughout.

Observations

QR Code Component (QrCode.tsx)

  • 👍 SVG rendering with rounded finder patterns and animated dot entrance looks great. prefers-reduced-motion respected properly.
  • ⚠️ The prevMatrixRef / renderCountRef pattern for animation diffing is clever but the useMemo depends on prefersReduced but mutates prevMatrixRef inside the memo. This works because refs are stable, but be aware that if prefersReduced changes, you get a re-render that treats all dots as "new" (animation replays). Minor, not blocking.
  • ⚠️ isLogoZone uses Math.ceil(size * 0.2) for logo modules. For larger QR versions (long URIs), verify the error correction headroom at ecc: "medium" — clearing center modules with a large payload could push past the ECC limit and make the QR unscannable on some readers.

Toast System (Toast.tsx)

  • 👍 Clean context-based approach. Timer cleanup on unmount is correct.
  • Nit: nextId is a module-level counter — fine for this use case, but in strict mode with double-mount (React 18 dev), IDs will skip. Not a real issue in production.
  • Nit: Consider using crypto.randomUUID() or Date.now() if ID uniqueness across hot reloads matters.

SheetModal (SheetModal.tsx)

  • 👍 Haptic feedback on close is a nice touch.
  • The handleAreaStyle has cursor: grab — on mobile this is irrelevant (no cursor), but on desktop web it sets the right expectation. Fine.
  • The pill handle area is tap-to-close. Consider whether users might accidentally tap it. With initialBreakpoint={1} and backdropBreakpoint={0}, the Ionic modal already handles swipe-to-dismiss, so the tap-close on the handle area adds redundancy (which is fine for discoverability).

Receive Page

  • 👍 Address order in copy sheet follows the PR description (Unified > Lightning > Arkade > Bitcoin). Logical priority.
  • 👍 44px minimum tap targets on copy buttons with touch-action: manipulation — good mobile practice.
  • The minSwapAllowed() hint for Lightning is helpful UX. The "Lightning temporarily unavailable" warning when swaps time out is a good degradation pattern.

package-lock.json

  • This is a new 8,555-line file being added. Is the repo switching from pnpm-lock to npm lockfile? If intentional, might be worth noting in the PR description.

Content.tsxnoFade prop

  • Clean addition. The no-content-fade class presumably disables a scroll-fade effect. Make sure the CSS class is defined (should be in ionic.css based on the diff).

Security

No concerns — this is purely UI/UX. No key material, no transaction logic, no external API changes.

Verdict

Clean, well-structured UI work. The QR ECC headroom question is the only thing worth verifying before merge. Everything else is polish-tier.

Reminder: PR is explicitly blocked on LNURL landing first — do not merge until that dependency is met.

@bordalix
Copy link
Copy Markdown
Collaborator

bordalix commented Apr 3, 2026

Issue: when user selects/copies one address, the qrcode should change to that address.

Scenario: paying wallet only reads lightning invoices, user wants to have a qrcode with only the bolt11 to show to the paying wallet.

Fixed by 74ca2b7

@bordalix
Copy link
Copy Markdown
Collaborator

bordalix commented Apr 3, 2026

Incorrect buttons spacing

Screenshot 2026-04-03 at 12 57 46

Fixed by 8e5667d

Screenshot 2026-04-03 at 13 08 55

@arkanaai
Copy link
Copy Markdown

arkanaai bot commented Apr 3, 2026

🔍 Arkana Review (followup — new commits)

Previous review covered the base. This followup addresses changes since 92f94999.

New Changes Observed

QR Code component rewrite (QrCode.tsx):

  • Moved from GIF-based QR (via encodeQR → gif blob URL) to inline SVG with rounded dot modules, styled finder patterns, and a centered Ark logo. Good visual upgrade.
  • Ripple animation on dot appearance (distance-based delay) with prefers-reduced-motion respect — well implemented.
  • prevMatrixRef for diff-based animation on value changes — clever, avoids full re-render animation when only data changes.
  • Potential issue: logoColor uses var(--logo-color) but I don't see this CSS variable defined in the diff. Verify it's set somewhere (or falls back gracefully).

Toast system (Toast.tsx):

  • Custom toast replaces all useIonToast call sites (Text, Table, ExpandAddresses, Backup, Logs, ReceiveQRCode).
  • Top-positioned, dark bg, auto-dismiss in 2s, 250ms ease-out animations with reduced-motion fallback.
  • Timer cleanup on unmount — good.
  • nextId is module-scoped (not in state) — fine for this use case, but note it persists across hot reloads in dev.

SheetModal redesign:

  • Removed close button, added drag handle (pill). Visual elevation via Ionic CSS custom properties.
  • Now calls hapticLight() on dismiss — nice touch.
  • Uses NavigationContext to detect if navbar is present and adjusts bottom padding.

Receive flow restructure (ReceiveQRCode):

  • Now fetches addresses on mount (previously expected them from a prior screen). This enables skipping the amount screen.
  • Amount and copy are bottom sheets instead of separate pages.
  • BIP21 URI built even before QR display is triggered — the useEffect dependency on addressesLoaded is correct.
  • Address order in copy sheet: Unified > Lightning > Arkade > Bitcoin — matches the PR description.
  • minSwapAllowed() hint below QR for Lightning minimum — helpful UX.
  • Dev fast-path: VITE_DEV_NSEC skips boot animation and auth state race — setAuthState('authenticated') called before initWallet to prevent race.

CSS changes:

  • ion-modal now has explicit z-index: 200 !important — verify this doesn't conflict with other overlays (action sheets, alerts).
  • Dark mode gets distinct --toast-bg, --toast-color, --sheet-bg — good.
  • .no-content-fade class to opt out of scroll fade mask — used on receive page where the QR needs full visibility.
  • :focus-visible replacing :focus on .focusable — correct, prevents purple flash on tap while maintaining keyboard accessibility.

Concerns

  1. ion-modal z-index !important: This could cause stacking issues if multiple modals or action sheets are open simultaneously. Ionic manages z-index internally — overriding it globally might cause the amount sheet to appear behind an action sheet. Consider scoping this to .sheet-modal class instead.

  2. 9,342 additions: The bulk is package-lock.json changes. The actual code diff is reasonable, but the lock file update should be reviewed for unexpected dependency additions (I see framer-motion added — is it used? The animation in QrCode.tsx is CSS-based).

  3. Missing --logo-color variable: The QR code SVG references var(--logo-color) but the CSS changes only add --sheet-bg, --toast-bg, --toast-color. If --logo-color is undefined, the logo paths will render as the initial value (typically transparent or inherited).

  4. useReducedMotion hook: Referenced in QrCode.tsx but the hook file isn't in the diff. Confirm it exists in the codebase.

Blocked Status

PR body clearly states this is blocked by LNURL implementation. The "show QR first without amount" UX only works end-to-end once zero-invoice Lightning is available. Do not merge until that lands.

@arkanaai
Copy link
Copy Markdown

arkanaai bot commented Apr 3, 2026

Followup review (8e5667d — fix button alignment)

Good fix. Moves the action buttons (Amount / Copy / Share) out of the scrollable content area into <ButtonsOnBottom>, which pins them properly regardless of QR code + warning box height. The WarningBox stays inline with the QR content where it belongs.

Also picks up the re-imported ButtonsOnBottom that was previously dropped. Clean separation of scrollable content vs fixed actions.

No new concerns.

@bordalix
Copy link
Copy Markdown
Collaborator

bordalix commented Apr 3, 2026

Broken: when receiving assets it says "sats" instead of the asset token
In this case it should say "Requesting 12 BB"

Screenshot 2026-04-03 at 13 13 11

Fixed by 4955070

Copy link
Copy Markdown
Contributor

Kukks commented Apr 3, 2026

a QR code with no border margin makes reading by scanners very hard.

@arkanaai
Copy link
Copy Markdown

arkanaai bot commented Apr 3, 2026

🔍 Arkana PR Review — wallet#480

PR: feat(receive): redesign receive flow with improved sheets and toasts
Author: sahilc0 | Base: wt-navbar-root-only-20260325 (#474)


Summary

Major UX overhaul of the receive flow: QR shown immediately (skipping amount screen), new SVG QR renderer with animated dots and embedded Arkade logo, custom Toast system replacing useIonToast, redesigned SheetModal with drag handle, and new bottom sheets for amount/copy on the receive page. Blocked on LNURL zero-invoice support.

What looks good

  • Custom Toast system (Toast.tsx): Clean context-based design, proper cleanup of timers on unmount, prefers-reduced-motion respected. Well-structured.
  • QR SVG renderer (QrCode.tsx): Nice upgrade from GIF to SVG. Finder pattern rendering with rounded corners, logo zone clearing with proper ecc: "medium" to compensate for removed modules. Reduced-motion compliance for dot animations.
  • Accessibility improvements: :focus-visible instead of :focus (no purple flash on tap), 44px minimum tap targets on copy buttons, aria-label on interactive elements, touch-action: manipulation.
  • SheetModal refactor: Drag handle instead of close button is a better mobile pattern. Haptic feedback on dismiss.
  • Address ordering in copy sheet: Unified → Lightning → Arkade → Bitcoin — correct priority order.
  • Dev workflow: VITE_DEV_NSEC guard to skip boot animation race is a good DX improvement.

Questions / Concerns

  1. useEffect dependency arrays in ReceiveQRCode.tsx — Several effects use stale closure patterns:

    • Line ~83: useEffect depends on [svcWallet] but reads recvInfo from closure. If recvInfo changes between renders, the setRecvInfo call will overwrite those changes. Consider using the functional form: setRecvInfo(prev => ({ ...prev, boardingAddr, offchainAddr, ... })).
    • Same pattern in the payment listener effect (also [svcWallet]) — setRecvInfo({ ...recvInfo, satoshis: sats, receivedAssets }) captures a stale recvInfo.
  2. QR code useMemo dependencyrenderCountRef.current is mutated inside useMemo (line ~97). While refs don't trigger re-renders, mutating state inside useMemo is a React anti-pattern. The memo could be re-evaluated at unexpected times in concurrent mode. Consider moving the counter increment outside the memo.

  3. prevMatrixRef comparison — The animation logic compares previous and current QR matrices. Since encodeQR with the same input produces the same matrix, the animation will only trigger on actual value changes — good. But the isUpdate check (prevMatrix !== null && prevMatrix.length === size) means the first render never animates. Is that intentional? Might want an entrance animation on mount.

  4. Toast nextId as module-level variablelet nextId = 0 at module scope means IDs reset on hot reload in dev but persist across component unmount/remount. This is fine for production but could cause confusion in dev with HMR. Minor — just noting it.

  5. ion-modal z-index overridez-index: 200 !important in ionic.css is a broad override. If other Ionic modals or overlays are used elsewhere, this could cause stacking context issues. Worth documenting why 200 was chosen.

  6. Missing cleanup for createBtcToArkSwap promise — In the satoshis effect, createBtcAddress() calls createBtcToArkSwap which returns a promise. If the component unmounts before it resolves, there's no abort mechanism. The Promise.allSettled will still run its .then() and call state setters on an unmounted component. Consider an abort flag.

  7. Sheet modal onClick on handle area — The handle area div has cursor: grab but uses onClick for dismiss. This is slightly confusing UX — grab implies drag. The Ionic modal already handles swipe-to-dismiss via initialBreakpoint, so the click-to-dismiss on the handle might be unexpected.

Security

  • ✅ No key material exposure — addresses are fetched via existing getReceivingAddresses API
  • ✅ QR encoding uses ecc: "medium" — appropriate error correction for logo overlay
  • ✅ No new external dependencies (SVG rendering is all inline)
  • ✅ BIP21 URI construction unchanged from existing logic
  • DOMPurify already in deps — no raw HTML injection vectors

Cross-repo impact

  • None — all changes are wallet-internal UI. No SDK or protocol changes.

Verdict

Solid UX improvement. The stale closure issues in useEffect dependencies (#1) are the main concern — they could cause subtle bugs where address/amount state gets overwritten. The rest are minor polish items. Blocked on LNURL as noted in the PR description.

@bordalix
Copy link
Copy Markdown
Collaborator

bordalix commented Apr 3, 2026

Mobile asset receive is totally broken

Screen.Recording.2026-04-03.at.14.58.31.mov

@arkanaai
Copy link
Copy Markdown

arkanaai bot commented Apr 3, 2026

🔍 PR Review — feat(receive): redesign receive flow with improved sheets and toasts

Reviewer: Arkana (automated)
Commit: eaefd87


Summary

Large UX refactor: replaces the amount-first receive flow with a QR-first approach, adds custom toast system, redesigned sheet modals with drag handle, and a sophisticated SVG QR code renderer with animated dot transitions. Good accessibility improvements throughout.

✅ Positives

  • :focus-visible over :focus — correct fix, eliminates the purple flash on mobile tap while keeping keyboard a11y
  • Custom toast system — clean context-based approach, reduced-motion compliant, proper cleanup of timers
  • QR code SVG renderer — nice touch with the animated dots, finder pattern rendering, and logo overlay. The ecc: "medium" error correction compensates well for the center logo zone
  • Sheet modal redesign — drag handle UX is much better than close button for bottom sheets on mobile
  • 44px touch targets on copy buttons — good adherence to WCAG touch target guidelines
  • Address ordering (Unified > Lightning > Arkade > Bitcoin) makes sense for progressive enhancement

⚠️ Issues & Suggestions

1. Memory leak: QR GIF blob URLs never revoked (removed code)

The old code created blob URLs via URL.createObjectURL() without ever calling URL.revokeObjectURL(). The new SVG approach avoids this entirely — good.

2. package-lock.json committed — intentional?

This PR adds a new package-lock.json (8555 lines). The wallet repo typically uses pnpm (pnpm-lock.yaml). Having both lockfiles can cause confusion. If the project uses pnpm exclusively, this file should likely be .gitignored.

3. createBip21 called inside useEffect without being in deps (QrCode.tsx lines ~235, ~310)

createBip21 is defined as a regular function inside the component, so it captures stale closures. It references recvInfo, swapAddress, invoice, lnurlSession, and several context values. The second useEffect that calls createBip21(amountInput) only has [amountInput] in its deps — it will use stale invoice, swapAddress, etc. Either wrap createBip21 in useCallback with proper deps, or inline the logic.

4. useEffect on [svcWallet] re-fetches addresses on every wallet change

useEffect(() => {
  if (!svcWallet) return
  if (boardingAddr && offchainAddr) { setAddressesLoaded(true); return }
  getReceivingAddresses(svcWallet)...
}, [svcWallet])

The early return guards against re-fetching, but boardingAddr and offchainAddr aren't in the dependency array. If svcWallet reference changes but addresses are already set, this works. But if addresses are cleared and svcWallet doesn't change, it won't re-fetch. Minor — just noting the implicit coupling.

5. Toast nextId is module-scoped global

let nextId = 0

This works fine in practice (single page app), but if ToastProvider is ever mounted/unmounted/remounted, IDs continue incrementing (never reset). Not a bug, just a design note — IDs only need uniqueness, not sequential continuity.

6. QR animation: renderCountRef used as React key suffix

key={`d-${row}-${col}-${renderCountRef.current}`}

This forces React to unmount/remount animated circles on every value change (even if the dot was already present). Intentional for re-triggering animation, but it means all new dots animate even if they were present before — the animation is "QR changed" not "this specific dot is new." The isNew check partially mitigates this, but the key change forces remount regardless. Consider using a stable key when !isNew.

7. setAuthState('authenticated') before initWallet completes (wallet.tsx)

setAuthState('authenticated')
await initWallet(privateKey)

In dev mode, auth state is set to authenticated before the wallet is actually initialized. If initWallet fails, the app thinks it's authenticated but has no wallet. The error is caught and logged, but authState isn't reverted.

8. Navigation hardcoded to Pages.Wallet

<Header text='Receive' back={() => navigate(Pages.Wallet)} />

Previously used the generic back prop. If ReceiveQRCode is reached from the asset detail page, back now always goes to Wallet instead of back to the asset. This changes the navigation behavior.

9. Sheet modal z-index override

ion-modal {
  z-index: 200 !important;
}

The !important on z-index is aggressive. If other modals (action sheets, alerts) use Ionic's default z-indexing, this could cause stacking issues. Consider scoping this to a specific class.

Security

  • ✅ No private key handling changes (dev-mode NSEC is gated behind import.meta.env.DEV)
  • ✅ No changes to transaction signing, VTXO handling, or exit paths
  • copyToClipboard usage is consistent and doesn't expose sensitive data
  • ✅ QR code generation uses the same encodeBip21 / encodeBip21Asset functions

Verdict

Solid UX improvement. The QR-first flow is the right direction, and the accessibility work is appreciated. The stale closure in createBip21 (issue #3) and the hardcoded back navigation (issue #8) are the most impactful items to address. The package-lock.json addition should be clarified.

PR is explicitly blocked on LNURL implementation — makes sense, the amountless QR is a degraded experience without zero-invoice Lightning.

@bordalix
Copy link
Copy Markdown
Collaborator

bordalix commented Apr 3, 2026

ReceiveAmount page is no longer used, can be removed.
Fixed by 0062dad

@arkanaai
Copy link
Copy Markdown

arkanaai bot commented Apr 3, 2026

🔍 Arkana PR Review — arkade-os/wallet#480

Scope: UI/UX redesign of the receive flow — new QR code component, sheet modals, toast system, and flow simplification.

Overview

This PR removes the ReceiveAmount intermediate screen and goes directly to ReceiveQRCode, which now handles address fetching, amount setting (via sheet modal), and address copying (via sheet modal) all in one place. A new custom Toast system replaces useIonToast across the codebase. The QR code renderer is rebuilt from scratch as an SVG with animated dots, rounded finder patterns, and an embedded logo.

Positive Observations

  1. Accessibility improvements are solid: :focus-visible instead of :focus, 44px tap targets on copy buttons, aria-label attributes, reduced-motion compliance across both QR and toast animations.
  2. The toast system (Toast.tsx) is clean — context-based, handles enter/exit animations, auto-dismisses, and properly cleans up timers on unmount.
  3. QR code component is significantly better than the GIF-based approach — SVG is sharper, scalable, and the dot-in animation with radial delay is a nice touch.
  4. Sheet modal redesign with the drag handle (Vaul-inspired) is more intuitive than the close button approach.
  5. Dev workflow improvementVITE_DEV_NSEC skip for boot animation and auth state fast-path is practical.

Issues & Suggestions

Medium Priority

  1. package-lock.json committed to a pnpm project — The wallet uses pnpm (engines field says pnpm >= 8), so committing a package-lock.json (npm lockfile) is likely unintentional and could cause confusion. If the project uses pnpm, this file should probably be in .gitignore. This adds ~8500 lines of noise.

  2. QrCode.tsxprevMatrixRef never resets on unmount — The ref accumulates state across mounts. If the component unmounts and remounts with the same value, prevMatrix will match and no animation plays. Consider resetting prevMatrixRef.current = null in a cleanup effect, or accepting this as intended behavior for same-value re-renders.

  3. QrCode.tsxrenderCountRef monotonically increases — Used as a key suffix for animated dots (d-${row}-${col}-${renderCountRef.current}). This is fine for uniqueness, but React will never reuse these keys. Not a bug, just worth noting for React reconciliation awareness.

  4. Toast nextId is a module-level variable — If multiple ToastProvider instances are ever mounted (unlikely but possible in tests or nested routing), they share the same ID counter. Consider using a ref inside the provider instead:

    const idRef = useRef(0)
    const id = idRef.current++
  5. ReceiveQRCode.tsxcreateBip21 is called in both a useEffect and inline — The function reads from component state (recvInfo, swapAddress, invoice, etc.) but is not memoized. Each render recreates it. Since it is used in effects, consider wrapping it with useCallback with appropriate dependencies, or restructuring so the effect computes the value directly.

Low Priority / Nits

  1. Keyboard.tsxasset?.assetId guard — Changed from asset to asset?.assetId for both inputMode initialization and the secondary display toggle. Good fix — prevents treating an empty AssetOption (with assetId: "") as a valid asset.

  2. Removed comments in App.tsx — Several explanatory comments about boot animation flow were removed. The code is clear enough without them, but the original "If we land on any non-Loading page…" comment was useful context for the fly-up exit mode. Minor.

  3. SheetModal.tsxhapticLight on close — Nice touch for mobile feel, but consider gating on platform (not all users are on mobile/capacitor).

  4. CSS !important on ion-modal z-indexz-index: 200 !important is a smell. Verify this doesn not conflict with other overlays (action sheets, alerts).

  5. wallet.tsxsetAuthState("authenticated") before initWallet — In the dev auto-init path, auth state is set before the wallet initializes. If initWallet fails, the user appears authenticated with no wallet. The consoleError catches it but the state is left inconsistent. Consider resetting authState in the catch block.

Security

No security concerns — this PR is entirely UI/UX. No key handling, transaction signing, or sensitive data flow changes. The VITE_DEV_NSEC handling is gated behind import.meta.env.DEV, which is correct.

Cross-Repo Impact

  • Blocked by LNURL work (as documented in the PR description) — showing QR first without an amount means no Lightning option until LNURL zero-invoice support lands.
  • No SDK or server-side changes required.

Summary

Well-structured UX improvement with good accessibility practices. The main actionable items are: (1) remove the accidental package-lock.json, (2) consider the createBip21 memoization, and (3) handle the dev auto-init auth state edge case. Everything else is minor polish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants